feat!: copy middleware context inputs to prevent accidental mutation by zastrowm · Pull Request #2742 · strands-agents/harness-sdk

zastrowm · 2026-06-11T20:40:26Z

Description

Middleware previously received live references to the agent's internal state. A middleware that accidentally mutated the messages array, a toolSpec, or modelState would corrupt agent state for subsequent turns.

This PR makes middleware context inputs defensive copies across the public stages:

InvokeModelStage — messages, systemPrompt, toolSpecs, and toolChoice are all deep-copied. modelState is removed from the context entirely (snapshotted before the chain, written back after). invocationState remains shared by reference since hooks and tools write to it during streaming.

ExecuteToolStage — toolUse is deep-copied to prevent mutation of the parsed tool input in the assistant message. invocationState remains shared by reference.

AgentStreamStage — Removed from the public API. The context contract (copy vs. reference for args/options) isn't finalized, and shipping it with different guarantees than the other stages would be confusing. It continues to work internally.

Design rationale is documented in strands-ts/src/middleware/README.md.

Related Issues

Type of Change

Breaking change

Breaking Changes

modelState removed from InvokeModelContext
AgentStreamStage, AgentStreamContext, AgentStreamResult removed from public exports

Both are acceptable — middleware just shipped and has no external consumers yet.

Testing

23 tests in copy-on-input.test.ts covering isolation guarantees for all copied fields, shared-reference behavior for invocationState, modelState writeback semantics, and functional-style context passing.

I ran hatch run prepare

Checklist

I have read the CONTRIBUTING document
I have added any necessary tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
My changes generate no new warnings
Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Middleware now receives isolated copies of agent state rather than live references. Messages are deep-cloned via Message.clone(), systemPrompt via cloneSystemPrompt(), toolSpecs/toolChoice via structuredClone(), and invocationState via shallow copy. modelState is excluded from the context entirely and snapshotted/written-back outside the middleware chain so middleware cannot affect it at any point.

The project's eslint config uses an explicit globals allowlist that doesn't include structuredClone. Use the existing deepCopy utility instead.

github-actions · 2026-06-11T20:55:29Z

Assessment: Comment

Solid, well-tested defensive-copy change with a clear design rationale documented in the middleware README and PR description. The snapshot/writeback approach for modelState is a clean way to enforce the isolation contract. Two items are worth addressing before merge: an incomplete deep-copy in Message.clone() (metadata is shared by reference, which leaks through the isolation boundary), and the public-API process flag for removing modelState from InvokeModelContext.

Review Categories

Correctness: Message.clone() shares metadata (incl. nested custom) by reference, partially defeating the isolation guarantee this PR establishes.
Public API: InvokeModelContext is an exported type; the modelState removal should carry api/needs-review per the bar-raising process. The design itself is well-argued.
Behavior: modelState writeback is skipped if the model call throws — a subtle change from the previous live-store behavior; worth confirming intent.
Testing: Excellent coverage of messages/systemPrompt/toolSpecs/modelState; gaps remain for defined-toolChoice copying and message-metadata isolation. Minor: prefer whole-shape key assertion over not.toContain.

Nice work isolating middleware from agent internals — the functional-style test cases passing modified contexts to next() are a great touch.

ExecuteToolStage: deep-copy toolUse so middleware cannot mutate the model's parsed tool input in the assistant message. AgentStreamStage: shallow-copy array args so middleware cannot push/splice the caller's original array. invocationState is intentionally left as a shared reference across all stages — tools and hooks write to it and those mutations must appear on AgentResult.

github-actions · 2026-06-12T17:41:15Z

Re-review (round 5) — Assessment: Approve ✅

Reviewed 8f5766a8. The clone() fix is exactly the agreed direction — Message.fromMessageData(deepCopy(this.toJSON())) does a full MessageData round-trip, dropping the metadata special-case and closing the ToolUseBlock.input / JsonBlock.json reference leaks in one shot.

Verified locally on this HEAD:

The leak probe that failed in rounds 3–4 (mutate clone.content[0].input.nested.a → original) now passes — the original is untouched.
Full suite green: 97 tests pass (copy-on-input 21 + messages 76), no type errors.

All blocking items are resolved. The args boundary is documented, invocationState sharing is consistent across stages, and clone() is now a true deep copy. Nice work iterating through this.

One optional, non-blocking follow-up

The "message content blocks are deep copied" test (copy-on-input.test.ts:48) still asserts only instance identity (not.toBe). The code is correct now, but that assertion would still pass if a future change reintroduced a nested-input leak. Upgrading it to a nested-mutation assertion (mirroring the metadata-isolation test right below it) would guard the regression — worth doing whenever convenient, not a merge blocker.

The context contract for AgentStreamStage (copy vs. reference semantics for args/options) is not yet finalized. Rather than ship an inconsistent surface alongside the other stages' clear guarantees, keep it internal until we decide on the right approach.

github-actions · 2026-06-12T18:28:08Z

Re-review (round 6) — Assessment: Approve ✅

Reviewed 3d35b317 (remove AgentStreamStage from public API). This is a sound, conservative call — rather than ship AgentStreamStage with an unfinalized args/options copy-vs-reference contract that would be inconsistent with the other stages' guarantees, keeping it internal until the contract is decided is the right move. The execution is clean:

Removed only from the top-level public index.ts; still exported from the internal middleware/index.js barrel and wired into agent.ts (line 1125), so it keeps working internally.
@internal JSDoc added to AgentStreamStage/AgentStreamContext/AgentStreamResult plus a clear README section explaining the deferral.
PR description, Breaking Changes, and the api/needs-review label all reflect the change accurately.

Verified locally on this HEAD:

src type-check clean (exit 0).
Full unit suite: 3331 tests pass across 122 files, no type errors. (AgentStreamStage internal tests still resolve it via the internal path.)
The one tsc error in steering.test.node.ts is pre-existing/unrelated — it's an integ-test file this PR doesn't touch, doesn't use AgentStreamStage, and the integ project only type-checks against a built dist.

Two minor, non-blocking notes:

Doc nit + open thread to reconcile

Test count: the description says "23 tests in copy-on-input.test.ts" but the file currently has 21 it() blocks — worth syncing the number.
Open invocationState thread: @opieter-aws's thread on the README (here) is still open, and your "Agreed; pass by copy" reply seems to point the opposite way from the current code, which passes invocationState by reference everywhere (there's no { ...invocationState } spread left in src/). The code is internally consistent with the README's "shared by reference" section, so this isn't a code blocker — just worth resolving the thread so the intended contract is unambiguous before merge.

Still approving — the remaining items are documentation/discussion cleanup, not code defects.

github-actions Bot added the size/m label Jun 11, 2026

zastrowm temporarily deployed to auto-approve June 11, 2026 20:40 — with GitHub Actions Inactive

zastrowm had a problem deploying to auto-approve June 11, 2026 20:40 — with GitHub Actions Failure

github-actions Bot added area-hooks Features or requests that might be implementable via hooks area-agent Related to the agent class or general agent questions enhancement New feature or request typescript Pull requests that update typescript code strands-running labels Jun 11, 2026

fix: replace structuredClone with deepCopy for eslint compatibility

90d4f26

The project's eslint config uses an explicit globals allowlist that doesn't include structuredClone. Use the existing deepCopy utility instead.

github-actions Bot added size/m and removed size/m labels Jun 11, 2026

zastrowm temporarily deployed to auto-approve June 11, 2026 20:44 — with GitHub Actions Inactive

zastrowm had a problem deploying to auto-approve June 11, 2026 20:44 — with GitHub Actions Failure